-
Notifications
You must be signed in to change notification settings - Fork 15
Construct new BlockSkylineMatrix from BlockSkylineMatrix and UnitRanges #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Construct new BlockSkylineMatrix from BlockSkylineMatrix and UnitRanges #229
Conversation
Take an existing BlockSkylineMatrix (or BlockBandedMatrix), and select a sub-matrix by passing UnitRanges for the rows and columns to select, returning a BlockSkylineMatrix (or BlockBandedMatrix).
I think I'm going to want a feature like this (actually possibly, eventually, even a more complicated version where I'd allow Needs tests - I can probably work on some if there's interest in merging this PR. Question: I've attempted to make the data copying efficient, but it's pretty ugly, so possibly fragile. Maybe an experienced BlockBandedMatrices.jl developer can see a better way? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #229 +/- ##
==========================================
- Coverage 88.31% 85.23% -3.09%
==========================================
Files 11 11
Lines 1113 1172 +59
==========================================
+ Hits 983 999 +16
- Misses 130 173 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bi_start = findblockindex.(axes(A), (rows.start, cols.start)) | ||
bi_stop = findblockindex.(axes(A), (rows.stop, cols.stop)) | ||
|
||
first_block = Int64.(BlockArrays.block.(bi_start)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be Int
, not Int64
last_blockindices = Int64.(BlockArrays.blockindex.(bi_stop)) | ||
|
||
orig_blocksizes = blocksizes(A) | ||
orig_row_sizes = [bs[1] for bs ∈ view(orig_blocksizes, :, 1)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very questionable. I think you might want to be using blockaxes
or blocklengths
.
I'm not going to review the rest since it all seems overly complicated.
not guaranteed that the `data` for the result is a contiguous subset of `A.data`. | ||
""" | ||
function BlockSkylineMatrix(A::BlockSkylineMatrix{T,DATA,BS}, rows::UnitRange, | ||
cols::UnitRange) where {T,DATA,BS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want AbstractUnitRange
`rows` and `columns`. This function allocates new memory and copies data, because it is | ||
not guaranteed that the `data` for the result is a contiguous subset of `A.data`. | ||
""" | ||
function BlockSkylineMatrix(A::BlockSkylineMatrix{T,DATA,BS}, rows::UnitRange, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this is the right name. The comment isn't exactly clear what this doing.
Adding tests would make it clear what exactly this is doing. I think this seems overly complicated. I'm pretty sure you can accomplish the same thing in a couple of lines using |
Thanks for the pointers @dlfivefifty. I've been thinking harder about what exactly my use case is - I wrote a script to mock up the essential features, and now I think I understand better exactly what I want to do. I think you're right that this PR as it stands is not well-conceived. The original idea was that for some BlockSkylineMatrix My particular motivation is that I will have a BlockSkylineMatrix where the blocks are pretty large (maybe 1000x1000) but sparse (think of the overall matrix as something like a three-dimensional derivative stencil). It seems like too much work (and not necessarily efficient, as the indexing would get much more complicated) to have a specialised sparse matrix type with the exact structure of my matrix. A BlockSkylineMatrix where I use the known structure in one 'outermost' dimension to identify large blocks that I know are zero seems like a good compromise with relatively simple indexing but also giving a large reduction in memory usage compared to a dense My problem then is that I will want to do something like convert that sub-matrix
I guess I should probably be able to figure out points 2 and 3 given a solution to 1. Edited to add: the reason for wanting to work with block-columns instead of just iterating over non-zero entries is that I'm assuming the blocks are large, so creating a SparseMatrixCSC one entry at a time would be slow compared to working out the offsets, etc. for converting a block. |
It sounds like you want a If you need to know the block bandwidths we can add functionality for computing it from the I and J are you sure you don’t want to use block indexing like |
Take an existing BlockSkylineMatrix (or BlockBandedMatrix), and select a sub-matrix by passing UnitRanges for the rows and columns to select, returning a BlockSkylineMatrix (or BlockBandedMatrix).